Skip to content

Make candidate template migrations faster #18025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
May 15, 2025

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented May 14, 2025

This PR makes the migrations for templates much faster. To make this work, I also had to move things around a bit (so you might want to check this PR commit by commit). I also solved an issue by restructuring the code.

Performance

For starters, we barely applied any caching when migrating candidates from α to β. The problem with this is that in big projects the same candidates will appear everywhere, so caching is going to be useful here.

One of the reasons why we didn't do any caching is that some migrations were checking if a migration is actually safe to do. To do this, we were checking the location (the location of the candidate in the template). Since this location is unique for each template, caching was not possible.

So the first order of business was to hoist the isSafeMigration check up as the very first thing we do in the migration.

If we do this first, then the only remaining code relies on the DesignSystem, UserConfig and rawCandidate.

In a project, the DesignSystem and UserConfig will be the same during the migration, only the rawCandidate will be different which means that we can move all this logic in a good old DefaultMap and cache the heck out of it.

Running the numbers on our Tailwind Plus repo, this results in:

    Total seen candidates: 2 211 844
Total migrated candidates: 7 775 
               Cache hits: 1 575 700

That's a lot of work we don't have to do. Looking at the timings, the template migration step goes from ~45s to ~10s because of this.

Another big benefit of this is that this makes migrations actually safe. Before we were checking if a migration was safe to do in specific migrations. But other migrations were still printing the candidate which could still result in an unsafe migration.

For example when migrating the blur and the shadow classes, the isSafeMigration was used. But if the input was !flex then the safety check wasn't even checked in this specific migration.

Safe migrations

Also made some changes to the isSafeMigration logic itself. We used to start by checking the location, but thinking about the problem again, the actual big problem we were running into is classes that are short like blur, and shadow because they could be used in other contexts than a Tailwind CSS class.

Inverting this logic means that more specific Tailwind CSS classes will very likely not cause any issues at all.

For example:

  • If you have variants: hover:focus:flex
  • If you have arbitrary properties: [color:red]
  • If you have arbitrary values: bg-[red]
  • If you have a modifier: bg-red-500/50
  • If you have a - in the name: bg-red-500

Even better if we can't parse a candidate at all, we can skip the migrations all together.

This brings us to the issue in #17974, one of the issues was already solved by just hoisting the isSafeMigration. But to make the issue was completely solved I also made sure that in Vue attributes like :active="…" are also considered unsafe (note: :class is allowed).

Last but not least, in case of the !duration that got replaced with duration! was solved by verifying that the candidate actually produces valid CSS. We can compute the signature for this class.

The reason this wasn't thrown away earlier is because we can correctly parse duration but duration on its own doesn't exist, duration-<number> does exist as a functional utility which is why it parsed in the first place.

Fixes: #17974

Test plan

  1. Ran the tool on our Tailwind UI Templates repo to compare the new output with the "old" behavior and there were no differences in output.
  2. Ran the tool on our Tailwind Plus repo, and the template migration step went from ~45s to ~10s.
  3. Added additional tests to verify the issues in @tailwindcss/upgrade - brakes boolean operators in Vue files #17974 are fixed.

[ci-all] let's run this on all CI platforms...

This is a first step in improving the performance of the upgrade tool.
Essentially let's cache the incoming candidate to the outgoing migration
based on the given inputs.

Notes: this is technically going to be slower now, because the
`location` will be different for every candidate right now.

In the next commits we will make sure to drop the `location` from the
cache (increasing the cache hits) and checking whether the migration is
safe ahead of time.
Instead of checking whether a candidate migration is safe during each
migration step, we can do it all up front. To make things worse, we did
check the safety in some migrations, but not in others which means that
we could still be performing unsafe migrations.

The big issue with "unsafe" migrations is where we use very small
classes such as `blur` or `visible` and they can be used in non-Tailwind
CSS contexts. Migrating these will result in unsafe migrations.

The moment we use variants, modifiers, arbitrary values the chances that
these are actual unsafe migrations are so low. We can use that as a
heuristic to check whether a migration is safe or not.

When we are dealing with `blur`, `visible` or `flex` we can apply the
other, existing, safety checks.

Once we know that a migration is unsafe, we will skip the migration.

Bonus points: this also takes the `location` information out of the
cache, which means that the cache will be smaller and increase cache
hits tremendously.
Before even gathering context of the candidate's surroundings, we can
already look at the candidate itself to know whether a migration will be
safe.

If a candidate doesn't even parse at all, we can stop early.
If a candidate uses arbitrary property syntax such as `[color:red]`,
there is like a 0% chance that this will cause an actual issue..

and so on.
When we do this upfront, then other migrations don't have to worry about
whether a candidate is potentially _not_ in its minimal canonicalized
form.

Additionally, this allows us to cleanup the main migrate function
itself, because we don't have to re-parse and re-print if nothing
changed in the meantime.
This is now implicitly handled by the `migrateCanonicalizeCandidate`
migration because we are parsing and printing the candidate.
Except if the `attr` is a `class`
When you have something like:
```
if (!duration) {}
```

It could be that we migrate this to:
```
if (duration!) {}
```

Hopefully our safety checks do not do that though. But in the event that
they do, the `!duration` should not be converted to `duration!` because
that class simply doesn't exist.

It wil parse correctly as:
```
[
  {
    "kind": "functional",
    "root": "duration",
    "modifier": null,
    "value": null,
    "variants": [],
    "important": true,
    "raw": "!duration"
  }
]
```

And that's because the `duration-<number>` _does_ work and produces
output. But this on its won won't produce any output at all.

If that's the case, then we can throw away any migrations related to
this and return the original value.
@RobinMalfait RobinMalfait requested a review from a team as a code owner May 14, 2025 16:48
When a candidate doesn't parse, we throw it away. However, if you are in
a Tailwind CSS v3 project then it could be that we are dealing with
classes that don't exist in Tailwind CSS v4 and will be migrated later.

It could also be that we are dealing with legacy syntax, e.g.:
`tw__flex` if you have a custom variant separator.

This fixes that by only bailing in Tailwind CSS v4 and up.
Otherwise the normal `class` would also be considered invalid, which is
not what we want.
In order to properly parse the value, we already had to make sure that
`group` and `peer` exist in the framework during a Tailwind CSS v3
→ Tailwind CSS v4 migration.

We now also verify that the actual produced class is valid, for this we
use `@apply` internally, but because of the `return null` this was
considered invalid.

This fixes that by making sure that we have some declarations, in this
case I used a random key like `--phantom-class: group`.

Then to make sure that `group` and `group/foo` are considered distinct
classes, I also added the modifier to the equation.
The color is called `superRed` which is migrated to `super-red`, but we
used the color `red-superRed` and expected `red-super-red` which doesn't
exist in the theme and therefore wasn't migrated.
Comment on lines -122 to -149
for (let [fromCandidate, toCandidate, fromThemeKey, toThemeKey] of migrate(rawCandidate)) {
// Every utility that has a simple representation (e.g.: `blur`, `radius`,
// etc.`) without variants or special characters _could_ be a potential
// problem during the migration.
let isPotentialProblematicClass = (() => {
if (fromCandidate.variants.length > 0) {
return false
}

if (fromCandidate.kind === 'arbitrary') {
return false
}

if (fromCandidate.kind === 'static') {
return !fromCandidate.root.includes('-')
}

if (fromCandidate.kind === 'functional') {
return fromCandidate.value === null || !fromCandidate.root.includes('-')
}

return false
})()

if (location && isPotentialProblematicClass && !isSafeMigration(location)) {
continue
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now handled ahead of time before attempting to even migrate

Comment on lines -75 to -79
location?: {
contents: string
start: number
end: number
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main migrate function will still receive the location, but each individual migration will not

Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think this makes sense. The only thought I had while going through it commit-by-commit was the fact that we know some utilities from v3 wouldn't work in v4 ootb (we do extend the design system somewhere for that IIRC) but it seems like you addressed this by opting out of that check if the migration is 3 => 4. 👍

@RobinMalfait RobinMalfait enabled auto-merge (squash) May 15, 2025 11:15
@RobinMalfait RobinMalfait merged commit 1ada8e0 into main May 15, 2025
21 checks passed
@RobinMalfait RobinMalfait deleted the feat/make-candidate-migrations-faster branch May 15, 2025 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@tailwindcss/upgrade - brakes boolean operators in Vue files
2 participants